MB-72082: add more stats around vector search - #441
Conversation
maneuvertomars
left a comment
maneuvertomars
left a comment
There was a problem hiding this comment.
We can have a look on these. Rest all looks good.
| vectors: make([]float32, 0, dim*numVectors*vo.numDocs), | ||
| vecDocIDs: make([]uint32, 0, numVectors*vo.numDocs), |
There was a problem hiding this comment.
This prealloc is dim * numVectors * numDocs, but numVectors comes from the first doc only. A multi vector first doc in a large batch reserves GBs up front, and a big enough product overflows and panics in makeslice. Can we go back to dim*numVectors and let append grow it?
There was a problem hiding this comment.
panics in makeslice
i think the panic only happens when the system has run out of memory? and the only case where the batch size can be large is on the fast merge path, for which we anyways size appropriately keeping the batch size in mind as well (this is something i've conveyed to tushar as well for sizing calculator).
in rest of the case, the batch is pretty small and i think doing a prealloc of a rough estimate for the entire batch (so alloc only once) is a benefit overall since we don't waste on realloc+GC? because we know that the total size needed for this batch is a multiple of the number of sub vectors
maybe we can remove this later on based on what you find, since i do see the benefit via the stats.
| if totalVecFields > int(atomic.LoadUint64(&vo.stats.TotVecSectionFieldsIndexed)) { | ||
| atomic.StoreUint64(&vo.stats.TotVecSectionFieldsIndexed, uint64(totalVecFields)) |
There was a problem hiding this comment.
This load-then-store max isn't atomic, two concurrent merges can interleave and the smaller value wins. Also the Tot prefix suggests a counter, not a max, and the flush path never updates it. Either we can do atomic.AddUint64, or a CAS loop with a Max* name.
There was a problem hiding this comment.
I don't think we should use Max naming convention for this since the stat still indicates the total number of vector fields in this index.
| atomic.AddUint64(&zapStats.TotMergeDroppedDocs, droppedDocs) | ||
| atomic.AddUint64(&zapStats.TotMergeOutputDocs, totalInputDocs-droppedDocs) |
There was a problem hiding this comment.
These are incremented before the merge runs, so aborted/failed merges still count their docs and retries double count. TotMergeOutputDocs is also input-minus-drops, not actual output. Can we count these after success, using computeNewDocCount's result?
There was a problem hiding this comment.
updated this in the latest PR
| defer close(closeCh) | ||
| _, _, err := mergeSegmentBases([]*SegmentBase{sb}, []*roaring.Bitmap{nil}, | ||
| path, DefaultChunkMode, closeCh, nil, nil) | ||
| path, DefaultChunkMode, closeCh, nil, nil, sb.stats) |
There was a problem hiding this comment.
rewriteSegmentBase passes sb.stats into the merge machinery, so a persist (callback rewrite) inflates the vector-section merge counters while TotMerges* stays untouched. Should this path skip stats, or be counted as a persist?
There was a problem hiding this comment.
I think given that this codepath still uses merge path to do a bunch of rewrite of the segment content in encryption usecases, we should still increment the associated relevant downstream stats. We're still tracking the flush operation itself as part of the TotPersist* stats, and realistically speaking when encryption isn't used, i don't think it bumps up any stats apart from writing the footer?
4 participants
Uh oh!
There was an error while loading. Please reload this page.